Fix: Prevent infinite loop in Tag Processor in certain truncated documents #45537
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
georgeh
approved these changes
Nov 4, 2022
I think it was only introduced in 14.5.0 RC (which is where we found it), so I don't think we need a point release of the main plugin |
|
Will this also fix the issue we had with error logs getting polluted? |
…ments. Previously the Tag Processor class has performed unchecked arithmetic on the result of `strpos()` when looking to close out HTML comments and other special sections (CDATA, doctype-declaration). This led to a situation where by means of type-coercion we added an integer value to a `false` returned by the string function, setting the cursor in the tag processor to a low index in the document, and creating an infinite loop condition. In this patch we're checking the results of calling `strpos()` in those places to avoid the type error and abort from the processor if we fail to find the end of those associated document sections.
dmsnell
force-pushed
the
fix/tag-processor-infinite-loop
branch
from
cd626a9 to
f921a4e
Compare
I'm still not sure how that one came up and am going to do more investigation. It's possible they are related and this issue caused the other, but I'd like to have a good model explaining it before saying one way or the other. My guess is that it was this problem that put the parser into a weird state which triggered the excessive warnings. That is, the only way I think the warnings we were seeing about |
dmsnell
added a commit
that referenced
this pull request
Nov 16, 2022
Follows #45537 When parsing truncated HTML it was brought to our attention that when passing out-of-bounds indices to `strspn()` and `strcspn()` that the behavior is different before and after PHP8. We also realized that when we cleaned up the problems with `substr()` we left some indices without bounds checking and that led to a different flavor of the same problem. When parsing the following HTML we run into warnings when calling `strspn()` and `strcspn()`. For pre-PHP8 versions this also leads to an infinite loop while in later versions it simply omits a warning. ```php <!-- wp:gallery {"linkTo":"none"} --> <figure class="wp-block-gallery has-nested-images ... ``` In this patch we're adding proper bounds checking wherever we update the internal pointer in the Tag Processor to avoid any further out-of-bounds issues. While this patch fixes the core issue at stake, it's worth performing a more complete audit of the index usage throughout the class and consider internalizing the string methods to avoid version inconsistencies and provide a more robust mechanism for aborting when passing the end of the provided input document. Props to @aidvu for quickly identifying this issue.
fullofcaffeine
pushed a commit
that referenced
this pull request
Nov 16, 2022
Follows #45537 When parsing truncated HTML it was brought to our attention that when passing out-of-bounds indices to `strspn()` and `strcspn()` that the behavior is different before and after PHP8. We also realized that when we cleaned up the problems with `substr()` we left some indices without bounds checking and that led to a different flavor of the same problem. When parsing the following HTML we run into warnings when calling `strspn()` and `strcspn()`. For pre-PHP8 versions this also leads to an infinite loop while in later versions it simply omits a warning. ```php <!-- wp:gallery {"linkTo":"none"} --> <figure class="wp-block-gallery has-nested-images ... ``` In this patch we're adding proper bounds checking wherever we update the internal pointer in the Tag Processor to avoid any further out-of-bounds issues. While this patch fixes the core issue at stake, it's worth performing a more complete audit of the index usage throughout the class and consider internalizing the string methods to avoid version inconsistencies and provide a more robust mechanism for aborting when passing the end of the provided input document. Props to @aidvu for quickly identifying this issue.
fullofcaffeine
pushed a commit
that referenced
this pull request
Nov 25, 2022
Follows #45537 When parsing truncated HTML it was brought to our attention that when passing out-of-bounds indices to `strspn()` and `strcspn()` that the behavior is different before and after PHP8. We also realized that when we cleaned up the problems with `substr()` we left some indices without bounds checking and that led to a different flavor of the same problem. When parsing the following HTML we run into warnings when calling `strspn()` and `strcspn()`. For pre-PHP8 versions this also leads to an infinite loop while in later versions it simply omits a warning. ```php <!-- wp:gallery {"linkTo":"none"} --> <figure class="wp-block-gallery has-nested-images ... ``` In this patch we're adding proper bounds checking wherever we update the internal pointer in the Tag Processor to avoid any further out-of-bounds issues. While this patch fixes the core issue at stake, it's worth performing a more complete audit of the index usage throughout the class and consider internalizing the string methods to avoid version inconsistencies and provide a more robust mechanism for aborting when passing the end of the provided input document. Props to @aidvu for quickly identifying this issue.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Previously the Tag Processor class has performed unchecked arithmetic on the result of
strpos()when looking to close out HTML comments and other special sections (CDATA, doctype-declaration). This led to a situation where by means of type-coercion we added an integer value to afalsereturned by the string function, setting the cursor in the tag processor to a low index in the document, and creating an infinite loop condition.In this patch we're checking the results of calling
strpos()in those places to avoid the type error and abort from the processor if we fail to find the end of those associated document sections.Why?
We shouldn't trigger infinite loops 🙃
How?
Checking for proper types and error-return-values before assuming things worked the way we expect.
Testing Instructions
The following input should trigger the infinite loop in
trunkNote that the
...is part of the input that led to identifying this issue. This snippet was created when block-unaware code truncatedpost_contentand cut inside the block comment delimiter, making it look like a normal HTML comment.For a quick test, navigate to the Gutenberg directory and run the following script:
In
trunkthis should hang, which you can confirm by instrumentingparse_next_tag()just inside thewhile ( true )loop. In this branch the code should immediately complete, returningfalsefrom the second call to$p->next_tag().The unit tests should also continue passing.
cc: @griffbrad